Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(command/build): add a test for build command #408

Merged
merged 5 commits into from
Oct 20, 2018

Conversation

huangjj27
Copy link
Member

@huangjj27 huangjj27 commented Oct 13, 2018

this PR is to check if a minimal example fixture::js_hello_world can be built in the test.

it provides precondition for #396, and closing #414


Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

@huangjj27
Copy link
Member Author

huangjj27 commented Oct 13, 2018

As is expected, this PR would fail because we set CARGO_TARGET_DIR to wasm-pack/target,
so when we run_wasm_pack with a temporary project(wasm-pack/target/t/.tmpXXXXXX/wasm-pack), the outputted files that should be in tmp_project/wasm32-unknown-unknown would be located at wasm-pack/target/wasm32-unknown-unknown.

Besides, wasm-bindgen also try to find target at tmp_project:

wasm-bindgen failed to execute properly. stderr:

error: failed to read `target/wasm32-unknown-unknown/release/js_hello_world.wasm`
	caused by: 系统找不到指定的路径。 (os error 3) system can't  find the path

Pending #396 until this test is passed

@huangjj27 huangjj27 closed this Oct 13, 2018
@huangjj27 huangjj27 reopened this Oct 13, 2018
@huangjj27
Copy link
Member Author

Besides, here is a question: would it be able to modify the same CARGO_TARGET_DIR when multiple tests are running?

huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Oct 13, 2018
the test case would pass after solving rustwasm#408
@huangjj27
Copy link
Member Author

I found the related code: https://github.com/rustwasm/wasm-pack/blob/master/src/bindgen.rs#L131, it is dead code to a certain path and will not change according to $CARGO_TARGET_DIR.

one more concern is that if we share the same target directory, will it possible to change the target files when there is multiple test running?

@fitzgen
Copy link
Member

fitzgen commented Oct 16, 2018

  error: failed to read `target/wasm32-unknown-unknown/release/js_hello_world.wa
| 	caused by: No such file or directory (os error 2)
test build::it_should_build_js_hello_world_example ... FAILED

I hope this is not actually trying to read a .wa file, and this is just the output getting cut off and it really is a .wasm file.

Besides, here is a question: would it be able to modify the same CARGO_TARGET_DIR when multiple tests are running?

Cargo ensures that target dirs are used in a mutually exclusive way, so we are safe from races. Is that your concern?

I found the related code: https://github.com/rustwasm/wasm-pack/blob/master/src/bindgen.rs#L131, it is dead code to a certain path and will not change according to $CARGO_TARGET_DIR.

I don't see how it is dead code, can you explain some more?

To make this test pass, we should be using https://docs.rs/cargo_metadata/0.6.0/cargo_metadata/struct.Metadata.html#structfield.target_directory to get the target directory. We are already using that crate for getting hte workspace root:

let crate_root = cargo_metadata::metadata(Some(&manifest))
.map_err(|_| Error::CrateConfig {
message: String::from("Error while processing crate metadata"),
})?
.workspace_root;

huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Oct 16, 2018
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case
have to pass after a successful build.
the old code are hard coded path with "/", which may cause error
on windows, thus changing to use PathBuf.join.

fixing rustwasm#414
@huangjj27
Copy link
Member Author

huangjj27 commented Oct 17, 2018

I hope this is not actually trying to read a .wa file

It's a mis-copying.

I don't see how it is dead code, can you explain some more?

I means that the command try to find wasm files in the relative target directory while the wasms have been outputed at $CARGO_TARGET_DIR/wasm32-unknown-unknown

To make this test pass, we should be using https://docs.rs/cargo_metadata/0.6.0/cargo_metadata/struct.Metadata.html#structfield.target_directory to get the target directory.

I've used std::env::var("CARGO_TARGET_DIR") to get the target directory as is described in cargo-book/environment-variables. It seems to work fine. Must I change for cargo metadata?

@fitzgen
Copy link
Member

fitzgen commented Oct 18, 2018

I've used std::env::var("CARGO_TARGET_DIR") to get the target directory as is described in cargo-book/environment-variables. It seems to work fine. Must I change for cargo metadata?

Hm, I'm actually not sure. But @alexcrichton would know.

@alexcrichton
Copy link
Contributor

I think CARGO_TARGET_DIR is only read by cargo, not written, so it's not guaranteed to be set. Reading it from cargo metadata would be the most reliable strategy!

@huangjj27
Copy link
Member Author

huangjj27 commented Oct 18, 2018

@alexcrichton

so it's not guaranteed to be set.

No, it's not, so in the codes I match if the environment variable exists.

But I'll change it.

@huangjj27
Copy link
Member Author

huangjj27 commented Oct 18, 2018

It seems that the build outputs too much and takes a long time. I think we don't need to output logs from the wasm-pack command in sub-process, because if we get something wrong, we could get results from sub-processes' stdout:

---- bindgen::can_download_prebuilt_wasm_bindgen stdout ----
thread 'bindgen::can_download_prebuilt_wasm_bindgen' panicked at 'called `Result::unwrap()` on an `Err` value: Http { message: "when requesting https://github.com/rustwasm/wasm-bindgen/releases/download/0.2.21/wasm-bindgen-0.2.21-x86_64-pc-windows-msvc.tar.gz" }', libcore\result.rs:1009:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- webdriver::can_install_geckodriver stdout ----
Created fixture at C:\Users\34937\Documents\wasm-pack\target\t\.tmp5JG25s\wasm-pack
thread 'webdriver::can_install_geckodriver' panicked at 'assertion failed: webdriver::install_geckodriver(&fixture.path).is_ok()', tests\all\webdriver.rs:25:5

Besides not outputing sub-command's output in test improves the CI performence. What do you think about it? @fitzgen @alexcrichton

@fitzgen
Copy link
Member

fitzgen commented Oct 19, 2018

It looks like it is hung at the end of the CI run. Will try poking CI and seeing if that fixes it.

Ideally we would detect when stdout is not a real terminal and avoid doing the spinner and fancy output and just do the normal output. We have an issue on file somewhere...

@huangjj27
Copy link
Member Author

@fitzgen

Ideally we would detect when stdout is not a real terminal and avoid doing the spinner and fancy output and just do the normal output.

I think we can redirect the stderr to a null, for all test result output to stdout and corespondent error will output there too

@fitzgen fitzgen added this to the 0.6.0 milestone Oct 20, 2018
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @huangjj27 :)

@fitzgen
Copy link
Member

fitzgen commented Oct 20, 2018

I think we can redirect the stderr to a null, for all test result output to stdout and corespondent error will output there too

I'm not sure I follow your proposal, do you mean from the script command in .travis.yml or inside wasm-pack code itself?

@fitzgen fitzgen merged commit 600d02b into rustwasm:master Oct 20, 2018
@huangjj27
Copy link
Member Author

I'm not sure I follow your proposal, do you mean from the script command in .travis.yml or inside wasm-pack code itself?

I mean the script command. for example, cargo test --locked 2>test.log, if we don't need that output, I think cargo test --locked 2>/dev/null will work fine too.

@huangjj27 huangjj27 deleted the test-build-for-example branch October 20, 2018 12:45
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Oct 20, 2018
the test case would pass after solving rustwasm#408
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Oct 20, 2018
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case
have to pass after a successful build.
xmclark added a commit to xmclark/wasm-pack that referenced this pull request Oct 29, 2018
Change to INFO when alerting users about missing fields in Cargo.toml

Combine filed missing messages into one INFO line

Fix bad formating

Merge pull request rustwasm#394 from mstallmo/master

Change to INFO when alerting users about missing fields Cargo.toml
child: Always print everything to our output

Also don't buffer the child's stdout and stderr.

error: Add stdout to the `Error::Cli` variant

refactor: Return failure::Error instead of wasm_pack::error::Error

refactor: Import self and use full module path for failure

Use full module path for failure to be consistent since
it's used like that in other modules.

refactor: Return failure::Error instead of wasm_pack::error::Error

chore: Run rustfmt

chore: Run rustfmt

Merge pull request rustwasm#392 from fitzgen/child-process-and-output-management

Child process and output management
Merge pull request rustwasm#401 from drager/return-failure-error

Return `Result<T, failure::Error>` instead of `Result<T, wasm_pack::error::Error>`
v0.5.1

Merge pull request rustwasm#404 from rustwasm/0.5.1

v0.5.1
feat(website): bump vers

Merge pull request rustwasm#405 from rustwasm/website-update

feat(website): bump vers
test(command/build): add a test for build command

useing local wasm-bindgen

Fix typo in test function name for copying the README

bugfix(bindgen-target-dir): use PathBuf to join

the old code are hard coded path with "/", which may cause error
on windows, thus changing to use PathBuf.join.

fixing rustwasm#414

change for cargo_metadata

Merge branch 'master' into test-build-for-example
Merge pull request rustwasm#408 from huangjj27/test-build-for-example

test(command/build): add a test for build command
Merge pull request rustwasm#412 from mstallmo/typo-fix

Fix typo in test function name for copying the README
Merge branch 'master' into fix-canonical-paths-on-windows
this change is not related to this PR


use absolutize


remove unused use


cargo fmt
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Oct 30, 2018
the test case would pass after solving rustwasm#408
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Oct 30, 2018
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case
have to pass after a successful build.
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Nov 8, 2018
the test case would pass after solving rustwasm#408
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Nov 8, 2018
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case
have to pass after a successful build.
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Nov 17, 2018
the test case would pass after solving rustwasm#408
huangjj27 added a commit to huangjj27/wasm-pack that referenced this pull request Nov 17, 2018
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case
have to pass after a successful build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants